Consider server timezone on _get_timezone_offset instead of django's settings #886
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR updates the _get_timezone_offset method so that it leverages the server's timezone rather than Django's timezone settings.
- Removed dependency on django.utils.timezone
- Obtains the server time via aware_now() and retrieves its timezone using ZoneInfo
Co-authored-by: Copilot <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #886 +/- ##
==========================================
+ Coverage 88.07% 88.17% +0.09%
==========================================
Files 32 32
Lines 1006 1006
Branches 104 104
==========================================
+ Hits 886 887 +1
Misses 101 101
+ Partials 19 18 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| server_tz = timezone.get_current_timezone() | ||
| server_time = aware_now() | ||
| # Use server_time.tzinfo directly if it is already a ZoneInfo instance | ||
| server_tz = server_time.tzinfo if isinstance(server_time.tzinfo, ZoneInfo) else ZoneInfo(str(server_time.tzinfo)) |
There was a problem hiding this comment.
The tzinfo class does not define __str__. It would be better to use server_time.tzinfo.tzname() which is guaranteed to exist.
There's no guarantee ZoneInfo will know what to do with the tzname(), though, so this won't work with obscure timezone implementations. (Edit: or with obscure zones not known to ZoneInfo. Either of these cases will probably break Django, so I don't think that this is a problem, just pointing it out.)
There was a problem hiding this comment.
I would request to come with a better solution for this in a separate PR, as going to merge this for a hhotfix
There was a problem hiding this comment.
this change caused our environment to stop dispatching tasks #894 - this should not have been a patch upgrade
There was a problem hiding this comment.
since you state in the docs that changing the timezone needs manual work https://django-celery-beat.readthedocs.io/en/latest/#important-warning-about-time-zones
There was a problem hiding this comment.
@alirafiei75 can you also have a look into this please?
There was a problem hiding this comment.
locally it is working - on the server, even if i update the server timezone to Europe/Berlin from UTC it doesn't work
There was a problem hiding this comment.
@FabianClemenz We wrote some tests to make sure that timezone differences do not affect the tasks running. Can you check them and if possible add a failing test so that I can check what is the expectation? (so then I can fix it)
There was a serious bug in 2.8 and some of the developers that had the problem reviewed the PR #879 and you can see the comments.
There was a problem hiding this comment.
@alirafiei75 i try to get time for that
For now
- On server
datereturns utc timezone - Going into docker container,
timezone.now()shows utc timezone,timezone.get_current_timezone()shows Europe/Berlin - this is also on containers working
datetime.now()shows Europe/Berlin
Maybe this line is the error? Getting the current time via datetime instead of timezone?
There was a problem hiding this comment.
localtime of beat shows time in Europe/Berlin - no matter what timezone the server is in
There was a problem hiding this comment.
Maybe this line is the error? Getting the current time via datetime instead of timezone?
This could not be the problem as it is not used anywhere else and just the timedelta matters and as long as both current_time and _last_full_sync are calculated in the same way, there won't be a problem.
|
@alirafiei75 can you please have another look |
I was out of town. Looks good to me. |
|
we got another PR in this front! #896 please check and let us know! |
DatabaseScheduler class consider's django timezone calculations to offset hour, but beat uses server's so that creates a situation where some tasks are excluded from running hours
Report: #885